re-add shadow DOM styles when reparented #2252
Conversation
| // Re-create stylesheet if the element is moved to a different window (by AppUI) | ||
| React.useEffect(() => { | ||
| window.addEventListener('appui:reparent', createStyleSheet); | ||
| return () => { | ||
| window.removeEventListener('appui:reparent', createStyleSheet); | ||
| }; | ||
| }, [createStyleSheet, latestShadowRoot]); |
There was a problem hiding this comment.
@GerardasB What do you think would be the best way to test this? It would be difficult to add a proper test for this in iTwinUI without manually recreating the custom event / reparenting logic.
I'm thinking we first manually verify that it works within the AppUI repo (by editing @itwin/itwinui-react in node_modules) and then after this is released, we can update the existing AppUI test scenario to ensure the styles look fine without any special handling on top of ProgressRadial.
There was a problem hiding this comment.
Sounds good, I'll verify if this change works and let you know.
There was a problem hiding this comment.
I can confirm, this works correctly in the reparenting use-case (I made sure to remove the custom useEffect workaround https://github.com/iTwin/appui/blob/b1c1d0c95a58ebbf2985bb2e61ad01d639e7f795/apps/test-app/src/frontend/appui/frontstages/TestPopoutFrontstage.tsx#L90)
However, this change breaks the regular use-case, where no reparenting is done and the component is simply mounted in a new window. (reparentPopoutWidgets=0 to configure in AppUI test-app: https://github.com/iTwin/appui/blob/b1c1d0c95a58ebbf2985bb2e61ad01d639e7f795/e2e-tests/tests/popout-widget.test.ts#L210)
This happens because useEffect that is scheduled in useLatestRef(shadowRoot) runs after the createStyleSheet called after ReactDOM4.flushSync(), which results in createStyleSheet returning early due to null latestShadowRoot.current.
There was a problem hiding this comment.
Thanks for testing it. I believe I've fixed the regular popout case in my latest changes. I'm passing the already existing shadow into createStyleSheet (rather than relying on useLatestRef).
I also added a basic test for it, since this test does not require us to reimplement any reparenting logic.
mayank99
force-pushed
the
mayank/fix-reparent-styles
branch
from
4dfb7c3
to
b5ee135
Compare
mayank99
requested review from
r100-stack and
smmr-dn
and removed request for
a team
Changes
This PR fixes #2230 by re-creating the
CSSStyleSheetinsideShadowRootwhenever theappui:reparentevent is emitted. This event was added in iTwin/appui#946 specifically to be consumed by ourShadowRootcomponent, in order to support the "reparent widget" feature (there is more info in iTwin/appui#893 if you're curious).It's not perfect and there is room for improvement, but it fixes the bug. The main issue right now is that we are unable to properly narrow down the reparented target, which means we are calling
createStyleSheetevery timeappui:reparentis dispatched anywhere in the window. We do have additional checks for stylesheet, so we're not unnecessarily recreating lots of stylesheets. Just something to be aware of and improve in the future.Testing
Since this is AppUI-specific functionality, it is hard for us to test directly. But it was confirmed to work in AppUI's own test app (see thread below).
I did add a small e2e test to ensure we are not regressing on the most basic use of popout.
Docs
Added changeset.